Hmm, I'm not sure what specific bug is fixed by this change? The geolocation should indeed be a string internally because float is imprecise, then we use the function Note.filter() to normalise the data when it needs to be compared.

My concern is whether changing how the geolocation is represented could incorrectly trigger synchronisation of existing notes.

I'm not sure what specific bug is fixed by this change

Consider that we have a note, we serialize it, and later unserialize it. The Note object we end up with should be identical to the one we started with. The bug is that it isn't.

The geolocation should indeed be a string internally because float is imprecise

The bug is that after serialize/unserialize, the geolocation is no longer represented internally as a string. So notes on either end of a sync have different representations.

Before

For example, without the proposed fix, it looks like this :

Note fields before serialization:

  latitude: '-53.10450000',
  longitude: '73.51710000',
  altitude: '0.0000',

Note fields after unserialization:

  altitude: 0,
  longitude: 73.5171,
  latitude: -53.1045,

The problem is the fields are converted from strings to floats

After

With the fix proposed in this PR, the fields are restored as strings after unserialization.

Note fields after (fixed) unserialization:

  altitude: '0.0000',
  longitude: '73.51710000',
  latitude: '-53.10450000',

Hmm, I don't know. When I've implemented this, I remember spending quite a bit of time to make sure geolocation is stable (at first it wasn't) and doesn't get resynced when a note is unserialised/serialised.

My concern is whether sync is going to be impacted by this change in unseralisation. As you said, before it was unserialised as a float "73.5171", now as a string "73.51710000". Maybe that's fine, or maybe not, but I'm having trouble evaluating the impact of this change.

Temporarily closing until I get time to come back to investigate and address comments.

My understanding is that notes are flagged for sync when they are saved, so a change in unserialise format has no effect. This is the case for all uses of unserialise: in sync, export/import RAW, external editting and encryption.

Perhaps the only way to confirm this is to do a manual test:

• use the current version of Joplin and sync a note between a "local" and "remote" client
• check that the geo data formats are different for both notes:
  • local note uses string
  • remote note uses float
• use the "fixed" version of Joplin (from this PR) and modify the note body on the "local"
• sync the change to the remote
• check
  • the remote note is updated and now uses string for geo data
  • there is no conflict, and that the "remote" does not try to sync back to the "local"

Would that be sufficient to prove the change is benign? Otherwise perhaps we just leave it as is.

I had a look at it again into more details, and can't see any issue with it, so let's merge. Thanks @mic704b!